Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

out_forward plugin can set targets dynamically via service discovery plugin. #2541

Merged
merged 25 commits into from
Oct 1, 2019

Conversation

ganmacs
Copy link
Member

@ganmacs ganmacs commented Aug 5, 2019

Which issue(s) this PR fixes:
Fixes #756 ?

What this PR does / why we need it:

I Added service_discovery plugin which sets config of servers in out_forwardy dynamically.
In this PR, I supported sd_file plugins and sd_static plugin.
sd_file checks the specified file periodically and update its targets.
sd_static is same as <server> configuration. So the following configurations are completely the same.

<match test>
  @type forward

  <server>
    host 127.0.0.1
    port 24224
  </server>
<match>
<match test>
  @type forward

  <service_discovery>
    @type static
    host 127.0.0.1
    port 24224
  </service_discovery>
<match>

Docs Changes:

Needed for service discovery plugin.

Release Note:

out_forward plugin can set targets dynamically via service discovery plugin.

Copy link
Member

@repeatedly repeatedly left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we create service_discovery helper for supporting create/configure/manage routines?


module Fluent
module Plugin
class SdFile < ServiceDiscovery
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use FileServiceDiscovery better

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

57d0ef0 fixed

)
end
rescue KeyError => e
raise Fluent::ConfigError, e
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe, KeyError's error message is not user friendly.
Adding more information is better.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added information about what config needs
61a05cd
b3f4ee905


module Fluent
module Plugin
class SdStatic < ServiceDiscovery
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

StaticServiceDiscovery is better

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

57d0ef0 fixed

def configure(conf)
super

@services << ServiceDiscovery::Service.new(:static, @host, @port, @name, @weight, @standby, @username, @password, @shared_key)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this plugin support only one server unlike out_forward?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch.. changed to be able to handle multiple services.
be04758

end
end

def select_node
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

select_node naming is ok?
This seems out_forward's one. select_service is better than select_node?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right. this is out_forward's name. changed to select_service

6a97db7

@ganmacs
Copy link
Member Author

ganmacs commented Aug 7, 2019

Should we create service_discovery helper for supporting create/configure/manage routines?

fixed 9883173

@@ -138,6 +139,10 @@ class ForwardOutput < Output
config_param :weight, :integer, default: 60
end

config_section :service_discovery do
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This define should be done via helper. See parser helper implementation.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed 54e8d53

# @param configurations [Hash] hash which must has discivery_service type and its configuration like `{ type: :static, conf: <Fluent::Config::Element> }`
# @param load_balancer [Object] object which has two methods #rebalance and #select_service
# @param custom_build_method [Proc]
def create_service_discovery_manager(title, configurations:, load_balancer: nil, custom_build_method: nil, interval: 3)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

service_disovery_create_manager is better to follow other plugin helpers.
helpers uses its name as function prefix.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed 46e6e0c

@_plugin_helper_service_discovery_title = title
@_plugin_helper_service_discovery_iterval = interval

@discovery_manager = Fluent::PluginHelper::ServiceDiscovery::Manager.new(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

unlike other helpers, this helper doesn't allow multiple instance, right?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You mean multiple @dicovery_mangers? if so, yes.
This manager instance should be one per output/input plugin. the manager has multiple sd plugins.

module Fluent
module Plugin
class ServiceDiscovery # already loaded
DiscoveryMessage = Struct.new(:type, :service) do
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Exposing this struct is needed? ServiceDiscovery.service_in/service_out also looks good for me.
With this approach, we can move this implementation into service_discovery.rb and users don't need to require additional file.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed 4ba7c7a

@repeatedly
Copy link
Member

@cosmo0920 Do you need to review this? You link this PR from elasticsearch plugin.

@cosmo0920
Copy link
Contributor

I'm considering to use service discovery plugin in elasticsearch plugin.
This PR looks good to me. 👍

@cosmo0920 Do you need to review this? You link this PR from elasticsearch plugin.

Curently, no. The above link is just a note.

@repeatedly repeatedly added the feature request *Deprecated Label* Use enhancement label in general label Sep 3, 2019
Signed-off-by: Yuta Iwama <ganmacs@gmail.com>
Signed-off-by: Yuta Iwama <ganmacs@gmail.com>
Signed-off-by: Yuta Iwama <ganmacs@gmail.com>
Signed-off-by: Yuta Iwama <ganmacs@gmail.com>
add sd_file

Signed-off-by: Yuta Iwama <ganmacs@gmail.com>
Signed-off-by: Yuta Iwama <ganmacs@gmail.com>
if @rr is greater than the size of @weight_array by reducing
weight_array number, node can be nil

Signed-off-by: Yuta Iwama <ganmacs@gmail.com>
Signed-off-by: Yuta Iwama <ganmacs@gmail.com>
Signed-off-by: Yuta Iwama <ganmacs@gmail.com>
Signed-off-by: Yuta Iwama <ganmacs@gmail.com>
Signed-off-by: Yuta Iwama <ganmacs@gmail.com>
Signed-off-by: Yuta Iwama <ganmacs@gmail.com>
Signed-off-by: Yuta Iwama <ganmacs@gmail.com>
Signed-off-by: Yuta Iwama <ganmacs@gmail.com>
Signed-off-by: Yuta Iwama <ganmacs@gmail.com>
Signed-off-by: Yuta Iwama <ganmacs@gmail.com>
Signed-off-by: Yuta Iwama <ganmacs@gmail.com>
Signed-off-by: Yuta Iwama <ganmacs@gmail.com>
Signed-off-by: Yuta Iwama <ganmacs@gmail.com>
Signed-off-by: Yuta Iwama <ganmacs@gmail.com>
Signed-off-by: Yuta Iwama <ganmacs@gmail.com>
Signed-off-by: Yuta Iwama <ganmacs@gmail.com>
Signed-off-by: Yuta Iwama <ganmacs@gmail.com>
Signed-off-by: Yuta Iwama <ganmacs@gmail.com>
Signed-off-by: Yuta Iwama <ganmacs@gmail.com>
@repeatedly
Copy link
Member

Finally merged :)

@repeatedly
Copy link
Member

@ganmacs Could you update fluentd document for service discovery plugin?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Feature request or improve operations feature request *Deprecated Label* Use enhancement label in general
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Dynamic increasing/decreasing servers of out_forward
3 participants